Improvements for Relocatable Compilations #2978

dsouzai · 2018-09-18T15:04:26Z

This PR introduces changes in OMR required to enable optimizations that are currently implicitly disabled for relocatable compilations.

Issue tracking all the PRs: eclipse-openj9/openj9#2921

0xdaryl · 2018-09-18T15:10:52Z

Thanks for the contribution. This should be marked WIP until #2977 is merged.

mstoodle

I know it's WIP, but figured I'd comment anyway

mstoodle · 2018-09-27T20:53:11Z

compiler/x/codegen/DataSnippet.cpp

+                                                               (uint8_t *)TR::SymbolType::typeClass,
+                                                               TR_SymbolFromManager,
+                                                               cg()),  __FILE__, __LINE__, getNode());
+         }


maybe we should assert here if !cg()->comp()->getOption(TR_UseSymbolValidationManager) ? Otherwise, don't we know we're not relocating a class pointer?

Yeah that's a good idea; although before this code, we never created a DataSnippet for a class pointer under AOT - this code is now active under AOT because checkcast inlining was enabled.

mstoodle · 2018-09-27T20:59:47Z

compiler/optimizer/LocalOpts.cpp

-      if (comp()->compileRelocatableCode() && !comp()->getOption(TR_DisablePeekAOTResolutions))
+      if (comp()->compileRelocatableCode() &&
+          !comp()->getOption(TR_UseSymbolValidationManager) &&
+          !comp()->getOption(TR_DisablePeekAOTResolutions))


You need a more detailed commit message to cover these changes.

mstoodle · 2018-09-27T21:04:13Z

compiler/compile/VirtualGuard.cpp

@@ -196,8 +196,6 @@ TR_VirtualGuard::createVftGuard
   aconstNode->setInlinedSiteIndex(calleeIndex);
   aconstNode->setByteCodeIndex(0);

-   comp->verifySymbolHasBeenValidated(static_cast<void *>(thisClass));


It would be better to squash this change with the one in #2977 that introduced the function in the first place...

Yeah you're right, I was trying to avoid merge conflicts :) . I'll try and clean this up.

mstoodle

I think the primary things that need to be fixed now are to clean up the commit titles/messages and I would like to hear the argument around methods being relocated using the old mechanisms even though classes are in the new mechanism.

mstoodle · 2018-10-01T14:40:30Z

compiler/compile/OMRCompilation.hpp

+    * a data structure, looking at several different possible answers
+    * before finally deciding on one. For a relocatable compile, only
+    * the final answer is important. Thus, a heuristic region is used to
+    * ignore all of the intermediate steps in determining the final


Can you also put this comment into the commit message, please?

mstoodle · 2018-10-01T14:42:03Z

compiler/x/amd64/codegen/OMRMemoryReference.cpp

+               {
+               if (cg->comp()->getOption(TR_UseSymbolValidationManager))
+                  {
+                  cg->addExternalRelocation(new (cg->trHeapMemory()) TR::ExternalRelocation(displacementLocation,


Please add a better commit message than "Update relocation for ClassAddress" ... I should be able to get the gist of the change just by reading the title and the text without knowing the context.

mstoodle · 2018-10-01T16:29:51Z

compiler/x/codegen/X86BinaryEncoding.cpp

@@ -868,6 +868,7 @@ TR::X86ImmInstruction::addMetaDataForCodeAddress(uint8_t *cursor)

      if (getReloKind() != -1) // TODO: need to change Body info one to use this
         {
+         TR::SymbolType symbolKind = TR::SymbolType::typeClass;


again, this commit should have a stand-alone title and message.

mstoodle · 2018-10-01T16:34:43Z

compiler/optimizer/VPHandlers.cpp

@@ -1599,6 +1599,7 @@ TR::Node *constrainAload(OMR::ValuePropagation *vp, TR::Node *node)
               sym->isFinal()))
            haveClassLookaheadInfo = true;

+         bool allowForAOT = vp->comp()->getOption(TR_UseSymbolValidationManager);


Given only classes are actually covered properly by the symbol validation manager (methods still use the "old" approach)...have you convinced yourself that the enhanced knowledge about classes doesn't end up enabling an incorrect runtime state due to the methods derived from those classes? Or will that be protected because the queries on the resulting method won't let things get out of hand?

From what I've seen, TR_MethodPointer is only generated when dealing with Profiled Pointers, so it should be ok using the "old" approach due to the class chain + first class loaded from classloader validations. Additionally, TR_RamMethod seems to be used only (or more specifically, can only be used) for the method being compiled, so that too should be ok using the "old" approach. That said, I'm doing some runs now with these changes made to the appropriate places

- if (symbolKind == TR::SymbolType::typeClass && cg()->comp()->getOption(TR_UseSymbolValidationManager)) + if (cg()->comp()->getOption(TR_UseSymbolValidationManager))

since I can't think of any good reason why the "new" approach shouldn't working, Towards the end of this work, I did find a bug in this code that I fixed, so perhaps I won't see that crash now.

mstoodle · 2018-10-01T16:35:38Z

compiler/optimizer/VPConstraint.cpp

@@ -1215,8 +1215,9 @@ TR::VPClassType *TR::VPClassType::create(OMR::ValuePropagation *vp, const char *
   if (classObject)
      {
      bool isClassInitialized = false;
+      bool allowForAOT = vp->comp()->getOption(TR_UseSymbolValidationManager);


another commit that should have a more meaningful commit title and message

mstoodle · 2018-10-01T16:36:08Z

compiler/x/codegen/DataSnippet.cpp

@@ -54,9 +54,8 @@ TR::X86DataSnippet::addMetaDataForCodeAddress(uint8_t *cursor)
   if (_isClassAddress)
      {
      bool needRelocation = TR::Compiler->cls.classUnloadAssumptionNeedsRelocation(cg()->comp());
-      if (needRelocation)
+      if (needRelocation && !cg()->comp()->compileRelocatableCode())


another commit that should have a more meaningful title and message...These changes do not "Enable checkcast inlining under AOT"

needRelocation should really be renamed to assumptionNeedsRelocation ; I'll allow it in a subsequent PR.

mstoodle · 2018-10-01T16:49:58Z

compiler/optimizer/LocalOpts.cpp

@@ -8058,7 +8058,9 @@ TR_ColdBlockMarker::hasNotYetRun(TR::Node *node)
      TR::SymbolReference *symRef = node->getSymbolReference();
      bool isUnresolved;

-      if (comp()->compileRelocatableCode() && !comp()->getOption(TR_DisablePeekAOTResolutions))
+      if (comp()->compileRelocatableCode() &&
+          !comp()->getOption(TR_UseSymbolValidationManager) &&


Are you just trying to preserve the old behaviour here by overloading on TR_UseSymbolValidationManager? The downstream code doesn't seem dependent on the symbol manager to me?

Are you just trying to preserve the old behaviour here by overloading on TR_UseSymbolValidationManager?

Yeah that's exactly it. I wanted there to be a way for the old code to run exactly as it did without all the new changes.

Although... the isInterpretedForHeuristics change also affects the "old" behaviour... Though, it only affects it from a heuristic point of view and, if anything, results in better decisions being made, so I suppose (like 1e645c3) is just a generally good thing to have

mstoodle · 2018-10-01T16:52:08Z

compiler/compile/Method.cpp

@@ -385,6 +385,7 @@ bool         TR_ResolvedMethod::isPublic()                                 { not
 bool         TR_ResolvedMethod::isFinal()                                  { notImplemented("isFinal"); return false; }
 bool         TR_ResolvedMethod::isStrictFP()                               { notImplemented("isStrictFP"); return false; }
 bool         TR_ResolvedMethod::isInterpreted()                            { notImplemented("isInterpreted"); return false; }
+bool         TR_ResolvedMethod::isInterpretedForHeuristics()               { notImplemented("isInterpretedForHeuristics"); return false; }


...makes me wonder if ForHeuristics (or its other state) is a concept worth promoting more broadly...

For example, the isUnresolved() change you had to make in another commit...

mstoodle · 2018-10-01T17:00:02Z

As a general statement, it would be nicer to see these things propagate into the code base in smaller chunks and not all at once. I know Eclipse OpenJ9 is very keen to get these changes into their 0.11 release and these changes are needed to enable even bigger things in OpenJ9, but I suspect at least some of these commits could have been submitted earlier than the full batch was and could have been merged early (i.e. not all of these changes are connected to the new symbol validation manager).

Anyway, it would be nice to see these improvements to the code base flowing in via a more incremental and hopefully spread out process, is my hopeful request for future submissions.

If the user specifies higher method counts, don't delay relocation and make the scount the same as the count. Signed-off-by: Irwin D'Souza <[email protected]>

If the user specifies disableKnownObjectTable, the KNOT does not get created. However, some parts of the code assume the KNOT always exist. Signed-off-by: Irwin D'Souza <[email protected]>

Add new external relocation types to enable the use of the Symbol Validation Manager which will be used to validate relocatable compilations. Signed-off-by: Irwin D'Souza <[email protected]>

Signed-off-by: Irwin D'Souza <[email protected]>

Enable the infrastructure to allow inlining of abstract methods inlining in relocatable compilations. Signed-off-by: Irwin D'Souza <[email protected]>

This commit adds another field into relodata so that a downstream project can make use of it. These fields are generic data fields whose contents can vary based on the relocation record being generated. Signed-off-by: Irwin D'Souza <[email protected]>

Previously, because almost everything was "unresolved" under AOT, all calls would use the unresolved dispatch. However, with the new validations, very few unresolved pointers are only unresolved because of AOT. Therefore, generating an unresolved dispatch is incorrect. The solution to this problem is to assume, during an AOT compile, that all methods are interpreted. Thus, if a method is resolved, the compiler will generate a resolved interpreter dispatch snippet, which can patch the caller if/when the method is compiled. Signed-off-by: Irwin D'Souza <[email protected]>

Sometimes, when devirtualizing, the compiler keeps the call virtual, but uses a cpIndex of -1, which causes a problem during relocation. Thus this commit adds a check to not devirtualize calls based on a Front End query. Signed-off-by: Irwin D'Souza <[email protected]>

Add a query to allow the validation of targets to be inlined for relocatable compiles. Signed-off-by: Irwin D'Souza <[email protected]>

Under the option to use the Symbol Validation Manager, the correct way to acquire the address for classes or methods, is to go through the manager, since it maintains consistency with all the validations as well as relocations. This commit transmutes the TR_ClassPointer, TR_MethodPointer, and TR_RamMethod into a TR_SymbolFromManager relocation record in order to maintain consistency with the rest of the validations. Signed-off-by: Irwin D'Souza <[email protected]>

Under the option to use the Symbol Validation Manager, the correct way to acquire the address for classes is to go through the manager, since it maintains consistency with all the validations as well as relocations. This commit transmutes the TR_ClassAddress into a TR_SymbolFromManager relocation record in order to maintain consistency with the rest of the validations. Signed-off-by: Irwin D'Souza <[email protected]>

This commit allows, under AOT and the option to use the Symbol Validation Manager, some findClassInfoAfterLocking queries in order to provide the optimizer with more information that was previously unavailable. Signed-off-by: Irwin D'Souza <[email protected]>

If a data snippet contains a class address, this address needs to be relocated. This commit ensures that the necessary relocation record is created. Signed-off-by: Irwin D'souza <[email protected]>

Under AOT, the compiler assumes some classes are unresolved, even if they actually aren't. This is because some queries will return NULL if the class can't be remembered in the SCC. Some optimizations, such as cold block marker, are affected by whether or not a block contains an unresolved reference. In AOT, many more blocks are incorrectly marked cold. This commit attempts to remedy that. Signed-off-by: Irwin D'Souza <[email protected]>

Add query for when the compiler asks about whether a method is interpreted for heuristic reasons, and not for functional correctness reasons. Signed-off-by: Irwin D'Souzai <[email protected]>

Signed-off-by: Irwin D'Souza <[email protected]>

mstoodle · 2018-10-04T16:35:48Z

genie-omr build all

mstoodle · 2018-10-04T20:09:07Z

I have opened #3029 to start documenting work that should be happen going forward to tease OpenJ9 and OMR further apart in their mutual dependency in specifically the relocation infrastructure. With that in place, I think we can merge this one now.

This was referenced Sep 18, 2018

Improvements for Relocatable Compilations eclipse-openj9/openj9#2919

Merged

WIP: Improvements for Relocatable Compilations Part 3 #2979

Closed

fjeremic added the comp:compiler label Sep 18, 2018

dsouzai mentioned this pull request Sep 18, 2018

Improvements for Relocatable Compilations eclipse-openj9/openj9#2921

Open

28 tasks

dsouzai changed the title ~~Improvements for Relocatable Compilations Part 2~~ WIP: Improvements for Relocatable Compilations Part 2 Sep 18, 2018

dsouzai mentioned this pull request Sep 18, 2018

Improvements for Relocatable Compilations Part 1 #2977

Closed

vijaysun-omr self-assigned this Sep 19, 2018

dsouzai force-pushed the AOTImprovementsPart2 branch 4 times, most recently from 07bf128 to 4339001 Compare September 25, 2018 20:23

mstoodle requested changes Sep 27, 2018

View reviewed changes

dsouzai force-pushed the AOTImprovementsPart2 branch from 4339001 to c1e9686 Compare September 28, 2018 14:49

dsouzai changed the title ~~WIP: Improvements for Relocatable Compilations Part 2~~ Improvements for Relocatable Compilations Sep 28, 2018

mstoodle requested changes Oct 1, 2018

View reviewed changes

dsouzai force-pushed the AOTImprovementsPart2 branch from c1e9686 to 6245869 Compare October 1, 2018 19:14

Irwin D'Souza added 2 commits October 2, 2018 23:03

Fix Higher Method Counts for AOT

e3f3e09

If the user specifies higher method counts, don't delay relocation and make the scount the same as the count. Signed-off-by: Irwin D'Souza <[email protected]>

Check if KNOT is NULL

6eaccdb

If the user specifies disableKnownObjectTable, the KNOT does not get created. However, some parts of the code assume the KNOT always exist. Signed-off-by: Irwin D'Souza <[email protected]>

dsouzai force-pushed the AOTImprovementsPart2 branch from 6245869 to c1b5b05 Compare October 3, 2018 17:13

Irwin D'Souza added 9 commits October 4, 2018 10:40

Add new External Relocation Types

5e15900

Add new external relocation types to enable the use of the Symbol Validation Manager which will be used to validate relocatable compilations. Signed-off-by: Irwin D'Souza <[email protected]>

Add option to guard uses of Sym Validation Manager

5f2e1cc

Signed-off-by: Irwin D'Souza <[email protected]>

Relocatable abstract method inlining

2347110

Enable the infrastructure to allow inlining of abstract methods inlining in relocatable compilations. Signed-off-by: Irwin D'Souza <[email protected]>

Add one more field for relodata

7ba167c

This commit adds another field into relodata so that a downstream project can make use of it. These fields are generic data fields whose contents can vary based on the relocation record being generated. Signed-off-by: Irwin D'Souza <[email protected]>

Add query to validate targets to be inlined

a18a64a

Add a query to allow the validation of targets to be inlined for relocatable compiles. Signed-off-by: Irwin D'Souza <[email protected]>

Irwin D'Souza added 5 commits October 4, 2018 10:44

Add relocation for class address in a data snippet

e5c1d42

If a data snippet contains a class address, this address needs to be relocated. This commit ensures that the necessary relocation record is created. Signed-off-by: Irwin D'souza <[email protected]>

Add isInterpretedForHeuristics

d96f3ba

Add query for when the compiler asks about whether a method is interpreted for heuristic reasons, and not for functional correctness reasons. Signed-off-by: Irwin D'Souzai <[email protected]>

Add option to enable/disable sym validation manager

e0bc346

Signed-off-by: Irwin D'Souza <[email protected]>

dsouzai force-pushed the AOTImprovementsPart2 branch from c1b5b05 to e0bc346 Compare October 4, 2018 14:46

vijaysun-omr assigned mstoodle and unassigned mstoodle and vijaysun-omr Oct 4, 2018

mstoodle approved these changes Oct 4, 2018

View reviewed changes

mstoodle mentioned this pull request Oct 4, 2018

Collection of pending improvements for relocation infrastructure #3029

Open

mstoodle merged commit 70c42dc into eclipse-omr:master Oct 4, 2018

jdmpapin mentioned this pull request Nov 20, 2018

Make SVM generate few enough records to enable it during startup eclipse-openj9/openj9#3752

Closed

28 tasks

dsouzai deleted the AOTImprovementsPart2 branch January 4, 2021 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements for Relocatable Compilations #2978

Improvements for Relocatable Compilations #2978

dsouzai commented Sep 18, 2018 •

edited

Loading

0xdaryl commented Sep 18, 2018

mstoodle left a comment

mstoodle Sep 27, 2018

dsouzai Sep 28, 2018

mstoodle Sep 27, 2018

mstoodle Sep 27, 2018

dsouzai Sep 28, 2018

mstoodle left a comment

mstoodle Oct 1, 2018

mstoodle Oct 1, 2018

mstoodle Oct 1, 2018

mstoodle Oct 1, 2018

dsouzai Oct 1, 2018

mstoodle Oct 1, 2018

mstoodle Oct 1, 2018

mstoodle Oct 1, 2018

mstoodle Oct 1, 2018

dsouzai Oct 1, 2018

dsouzai Oct 1, 2018 •

edited

Loading

mstoodle Oct 1, 2018

mstoodle commented Oct 1, 2018

mstoodle commented Oct 4, 2018

mstoodle commented Oct 4, 2018

Improvements for Relocatable Compilations #2978

Improvements for Relocatable Compilations #2978

Conversation

dsouzai commented Sep 18, 2018 • edited Loading

0xdaryl commented Sep 18, 2018

mstoodle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoodle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsouzai Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoodle commented Oct 1, 2018

mstoodle commented Oct 4, 2018

mstoodle commented Oct 4, 2018

dsouzai commented Sep 18, 2018 •

edited

Loading

dsouzai Oct 1, 2018 •

edited

Loading